-
Notifications
You must be signed in to change notification settings - Fork 24
Add standard retry mode #545
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: standard-retries
Are you sure you want to change the base?
Add standard retry mode #545
Conversation
fc2a957 to
56af01b
Compare
jonathan343
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Alessandra!
Looks great so far. I have a few suggestions below.
56af01b to
006c5e2
Compare
nateprewitt
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like we're off to a good start. I left some high-level feedback, but let me know if I can clarify anything!
| self._retry_quota = StandardRetryQuota() | ||
|
|
||
| async def acquire_initial_retry_token( | ||
| self, *, token_scope: str | None = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does this take token_scope but not use it? That's pointing to a possible miss in the implementation or our abstraction. We want to avoid having dead parameters as much as possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense. I added it because it's in the RetryStrategy interface and assumed we had to match the signature. SimpleRetryStrategy also ignores it and documents it the same way, so I followed that pattern.
Should we remove it from the interface entirely or is there a use case where we'd want this param?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can leave it as is for the moment, but let's talk to Jordon about long term intent. I'll need to go look at the reference architecture again to see if there's info there. It seems like we should have our reasoning in the design but it's not there right now. That'll help future implementers both on our team and externally.
Co-authored-by: Nate Prewitt <[email protected]>
Description of changes:
Adds support for a new retry mode:
standard. This behavior is now consistent with other AWS SDKs implementing standard mode. This mode also adds support for retry quotas which control how many unsuccessful retries a client can make.By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.